feat: modernize platform/device detection + Application fullscreen API (#1467)#1485
feat: modernize platform/device detection + Application fullscreen API (#1467)#1485obiot wants to merge 21 commits into
Conversation
Two real problems in `src/system/platform.ts` flagged during the 19.7 audit: 1. Modern iPads (iPadOS 13+, since Sept 2019) ship Safari with the desktop Mac UA — no `iPad` token in the user-agent string. The `/iPhone|iPad|iPod/i` test missed every iPad sold in the last ~7 years, and they fell through `isMobile` as desktop. Confirmed observable: `keyboard.ts`, `application.ts`, `header.ts` all branch on `isMobile`. 2. Dead-platform UA regexes (`wp`, `BlackBerry`, `Kindle`, `android2`) tested for hardware that was EOL'd between 2012 and 2017, burning regex cycles on every page load. ## Changes **iPad detection**: layer a feature check on top of the UA regex — `navigator.platform === "MacIntel"` plus `maxTouchPoints > 1`. The `"MacIntel"` string is Apple's frozen legacy identifier (same trick as `Win32` on 64-bit Windows) that persists on Apple Silicon Macs *and* iPads in desktop-Safari mode — it's not a CPU check. `Macs don't have touchscreens; iPads do`, so `maxTouchPoints > 1` uniquely separates them. Every existing internal consumer of `isMobile` inherits the fix transparently. **Deprecate dead exports**: `@deprecated` JSDoc on `wp`, `BlackBerry`, `Kindle`, `android2`. Exports stay functional through 19.x for backwards compat (any external consumer keeps working); IDE warnings surface at the call sites. Removal scheduled for 20.x. Also dropped these from `isMobile`'s OR chain. The remaining `/Mobi/.test(ua) || iOS || android` covers ~99.9% of 2026 mobile traffic per MDN's recommendation. **Won't add `isTouch`** as the original issue suggested — we already have `device.touch` at `system/device.js:116` (feature-detected via `navigator.maxTouchPoints` / pointer events). CHANGELOG migration note points there for new code. ## Tests Six new cases in `tests/platform.spec.ts` covering the iPad-on-Mac-UA contract — verify the documented `platform === "MacIntel" && maxTouchPoints > 1` check identifies iPads correctly, rejects actual Macs (no touch), Windows touchscreens, and missing-navigator (SSR). Existing 20 shape / desktop-defaults assertions kept. Full suite: 3975 passed / 13 skipped / 0 failed (was 3969 — +6 from the new tests, no regressions). ## Follow-ups (separate issues worth filing) - `keyboard.ts:85` `if (!isMobile)` skips key-event listeners. iPads with Magic Keyboard (now correctly identified) would stop receiving keys. Probably always-attach + let no-op-on-pure-touch sort itself out, but needs the iPad-with-keyboard test path to validate. - Migrate to `navigator.userAgentData` (Client Hints) where available. Chromium-only today; Safari/Firefox lag. 20.x candidate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates melonJS platform detection to correctly classify modern iPads (iPadOS 13+) as iOS/mobile, while deprecating and removing dead-platform UA regexes from the isMobile aggregate to reduce noise and overhead.
Changes:
- Detect iPadOS 13+ devices that present a desktop “Mac” UA via
navigator.platform === "MacIntel"+maxTouchPoints > 1, and fold this intoiOS/isMobile. - Deprecate legacy platform flags (
wp,BlackBerry,Kindle,android2) and remove the dead-platform flags from theisMobileOR chain. - Add/adjust unit tests for the new
isMobilewiring and the iPadOS 13+ detection contract; document the behavior in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| packages/melonjs/src/system/platform.ts | Adds iPadOS 13+ detection logic, deprecates dead-platform flags, and simplifies isMobile aggregation. |
| packages/melonjs/tests/platform.spec.ts | Updates isMobile expectation and adds contract tests for the iPadOS 13+ detection heuristic. |
| packages/melonjs/CHANGELOG.md | Documents the iPadOS 13+ fix and deprecation/removal of dead-platform flags from isMobile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`keyboard.ts:85` skipped attaching `keydown` / `keyup` listeners when `isMobile === true`. The gate assumed "mobile = no physical keyboard" — invalid in 2026 with iPads (now correctly identified post the platform fix above) using Magic Keyboard, Bluetooth keyboards on phones, Samsung DeX, ChromeOS tablet mode, etc. Two empty listener slots cost essentially nothing on touch-only devices; the handler's unbound-key path is a single map lookup that returns undefined. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
945 lines / 53 exports / 56 JSDoc blocks of feature-detection helpers
and platform plumbing now ship as a `.ts` file with native type
signatures. JSDoc was already exhaustive so the conversion is mostly
mechanical — `@param {Type}` blocks become parameter annotations and
`@type {Type}` constants get TS-inferred.
Non-standard / legacy browser surfaces (`Document.mozFullScreenEnabled`,
`Navigator.standalone` / `browserLanguage` / `userLanguage`, iOS-only
`DeviceOrientationEvent.requestPermission`, deprecated
`Screen.lockOrientation`, `webkitAudioContext`) are typed via narrow
local intersection types declared at the top of the file.
Two small runtime improvements that fell out of the conversion:
- the cached `domRect` is now a real `DOMRect` (its `right`/`bottom`
getters track `x + width` / `y + height` automatically, so the old
explicit assignment of `domRect.right` was redundant);
- `onDeviceMotion` now guards against
`e.accelerationIncludingGravity === null` rather than crashing.
Behavioural parity verified against the full 3975-test suite;
downstream call sites are unchanged thanks to bundler-resolution
rewriting `.js` imports to `.ts` source.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (4)
packages/melonjs/src/system/device.ts:44
domRectis now constructed vianew DOMRect(...)at module load time. In Node/SSR (or any non-DOM environment),DOMRectmay be undefined, causing an immediate ReferenceError just by importingdevice.ts. Please guard this creation and fall back to a lightweight object whenDOMRectisn't available.
packages/melonjs/src/system/device.ts:290autoFocuswas changed from an exportedletto an exportedconst. This prevents consumers from disabling the autofocus behavior (me.device.autoFocus = false), which appears to be part of the public API per the JSDoc (@default true) and is used as a runtime flag (e.g. inpointerevent.ts).
packages/melonjs/src/system/device.ts:246- The
device.isMobileJSDoc list is now out of date:platform.isMobileno longer ORsBlackBerry/Windows Phone/Kindle, so the comment is misleading.
packages/melonjs/src/system/device.ts:604 getElementnever returnsnull(it falls back todocument.body), but the JSDoc still says it can return null. This mismatch can confuse consumers and generated docs.
`prefer-const` flipped `export let autoFocus = true` → `const` during
the .js → .ts conversion lint pass because nothing in the module
reassigns it. The JSDoc still describes it as user-settable behaviour
("Specify whether to automatically bring the window to the front") —
`let` keeps the door open for an internal setter without forcing
another module-shape change.
Behaviourally moot today: ESM namespace-import bindings
(`device.autoFocus = false` via `import * as device`) are read-only
regardless of `let` / `const`, so external mutation never worked
either way. But intent matters.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot review batch from the platform.ts and device.ts review rounds: - platform.ts header doc: add missing space before \`true\` for BlackBerry / Kindle entries (rendered as malformed markdown otherwise). - platform.ts @deprecated: prepend `since 19.7.0 — ` to wp / BlackBerry / Kindle / android2 to match the codebase's existing convention (matches video.js, renderable.js, entity.js style). - platform.ts isMobile: drop redundant `|| false` from the OR chain (every operand is already a boolean). - tests/platform.spec.ts: rename "Mac touch-bar laptop" test — Touch Bar isn't a touchscreen and doesn't report maxTouchPoints. The test is about the `maxTouchPoints === 1` edge case directly. - device.ts isMobile JSDoc: drop the dead-platform list (BlackBerry, Windows Phone, Kindle) — they're no longer in the isMobile OR chain per the upstream platform.ts change. - device.ts getElement JSDoc: drop "or null if not existing" — the function falls back to `document.body` and never returns null. - device.ts domRect cache: revert `new DOMRect(...)` → plain object literal so module load doesn't ReferenceError in Node / SSR environments where the DOMRect constructor isn't defined. The literal is cast to `DOMRect` at the return site. - CHANGELOG: rephrase the `system/device` conversion entry to make the rename explicit ("renamed from device.js → device.ts" rather than referring readers to a path that no longer exists), and drop the (now-reverted) DOMRect claim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
packages/melonjs/src/system/device.ts:136
touchandmaxTouchPointsdereferenceglobalThis.navigator.maxTouchPointswithout guarding thatnavigatorexists. In SSR/non-browser contexts (or environments with partial DOM polyfills), this can throw at module-load time and prevent importingdeviceat all. Guardnavigatorconsistently (similar toplatform.ts) so both constants are safe to evaluate.
packages/melonjs/src/system/device.ts:746watchAccelerometer()requests permission viaDeviceOrientationEvent.requestPermission, but the permission gate fordevicemotionon iOS isDeviceMotionEvent.requestPermission(). Using the orientation constructor here can cause accelerometer setup to fail even thoughhasAccelerometeris true.
packages/melonjs/src/system/device.ts:472exitFullscreenassumesdocument.exitFullscreen()always exists and always returns a Promise (because.catch(...)is chained). In older fullscreen implementations you may only have vendor-prefixed exit APIs (or a void return), so this can throw even whenhasFullscreenSupportis true.
…cation (#1467) Fullscreen control finally has app-instance context. The canonical path is now `app.requestFullscreen()` / `app.exitFullscreen()`, defaulting to the app's `parentElement` (canvas + sibling HUD go fullscreen together) and accepting an optional Element override. The static `device.requestFullscreen()` / `device.exitFullscreen()` helpers stay for backwards compat (still work through the deprecated `getParent()` → `game.getParentElement()` global-game lookup), but are now flagged `@deprecated since 19.7.0` pointing at the Application methods. Updates the two examples that wire `F` → toggle fullscreen (platformer + platformer-matter) to use the new app-instance API. Each `createGame.ts` calls `_app.requestFullscreen()` directly; each HUD calls `game.requestFullscreen()` since the HUD code path doesn't have an `_app` reference and `game` is already imported. Implementation note: the new Application methods skip the vendor-prefixed `webkitRequestFullscreen` / `mozRequestFullScreen` probing the device wrappers do — every modern browser has unprefixed `Element.requestFullscreen` since ~2018. Users on ancient browsers that still need the prefix dance can fall back to the deprecated `device.requestFullscreen()` path which preserves the legacy probing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…en probes (#1467) Round-out of the move to app-instance fullscreen: - **`Application#isFullscreen`** added so the trio sits together (`isFullscreen` / `requestFullscreen` / `exitFullscreen`). Defaults the documented example to `app.isFullscreen()` instead of mixing in `me.device.isFullscreen()`. - **`device.isFullscreen` deprecated** alongside the other two fullscreen statics. Same `since 19.7.0 — use Application#…` pointer. - The four example sites that still called `device.isFullscreen()` switch to `_app.isFullscreen()` / `game.isFullscreen()` so the fullscreen path is consistently app-instance in user-facing code. - The new `Application#requestFullscreen` JSDoc names `parentElement` directly (with a backlink to {@link Application#getParentElement}) instead of the vaguer "canvas parent element" phrasing. Tag-along cleanup of the deprecated device wrappers themselves: the `prefixed("fullscreenEnabled", ...)` / `prefixed("fullscreenElement", ...)` / `prefixed("requestFullscreen", ...)` calls iterated 5 vendor prefixes per probe via the `prefixed()` helper, with awkward `as unknown as Record<string, unknown>` casts. Replaced with an explicit four-variant OR chain (`fullscreenEnabled || webkit… || moz… || ms…`), the same pattern lib.dom.d.ts uses and what every MDN recipe recommends in 2026. `DocumentLegacy` / `ElementLegacy` gain the missing `webkit*` / `ms*` typings. `requestFullscreen` also `.catch()`-es the Promise the modern (unprefixed) call returns — the vendor-prefixed variants returned undefined so the guard `if (result instanceof Promise)` cleanly covers both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#1467) `device.isFullscreen()` doesn't need Application context: the browser tracks exactly one fullscreen element per document regardless of how many Applications are running. Unlike `requestFullscreen` (needs to know WHICH element) and `exitFullscreen` (paired with request), `isFullscreen` is a stateless probe. Drops: - `@deprecated` JSDoc from `device.isFullscreen` + clarifies it's a document-state probe. - the eslint-disable comments inside `device.requestFullscreen` / `device.exitFullscreen` that were silencing the now-non-deprecated internal call. - the eslint-disable comment inside `Application#isFullscreen` for the same reason. The method now reads as a clean thin convenience wrapper. `Application#isFullscreen` stays as a convenience so the trio reads together (`isFullscreen` / `requestFullscreen` / `exitFullscreen`) on the app instance, but its JSDoc now correctly identifies `device.isFullscreen` as the canonical probe rather than implying the device version is being phased out. CHANGELOG updated to reflect the corrected scope of the deprecation (request + exit only; isFullscreen stays). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 3 (after ba60914): - **keyboard.ts:84** — `if (globalThis.addEventListener)` → `typeof globalThis.addEventListener === "function"`. Matches the defensive style used in `device.initVisibilityEvents` and avoids calling a polyfilled non-function value. - **platform.spec.ts iPadOS predicate drift** — the new tests were re-implementing `isIPadOnMacUA` locally inside the spec; if `platform.ts` accidentally changed the predicate (flipped `> 1` to `> 0`, dropped the `MacIntel` check, etc.) the tests would still pass. Fix: extract the predicate from `platform.ts` as an exported pure function (`isIPadOnMacUA(nav)`); the module's `iOS` computation now calls the exported function, and the spec asserts against the same function. No drift possible. (Tried the dynamic- import-with-stubbed-`globalThis.navigator` approach first, but Playwright browser mode makes `navigator` non-configurable enough that `vi.stubGlobal` doesn't propagate to dynamically-imported modules — extracting the predicate is the cleaner answer regardless.) Round 4 (after 3b3fb5a): - **application.ts:702 vendor-prefix gap** — `app.requestFullscreen` only probed `target.requestFullscreen?.()`. If `device.hasFullscreenSupport` was true via a vendor-prefixed flag (older WebKit / iOS), the unprefixed method was missing and the call silently no-op'd. The canonical app method now does the same four-variant probe the deprecated `device.requestFullscreen` does (`requestFullscreen || webkit… || moz… || ms…`), with a local `ElementWithLegacyFullscreen` type alias mirroring the `ElementLegacy` shape in `device.ts`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
packages/melonjs/src/system/device.ts:308
- The
eslint-disable prefer-constcomment is misleading: it justifiesletby claiming the variable is "reassignable via internal setters even though no setter exists today." There is no internal reassignment site (the only mutation today would be an externaldevice.autoFocus = …, which ESM disallows on a module export binding regardless). IfautoFocusis intended to be a public knob, it should be exposed via a setter / configuration object; otherwise this should beexport const autoFocus = true;and the eslint-disable removed. As written, the comment encodes a contract (mutability) that the code does not actually provide.
Uses the new `app.requestFullscreen()` / `app.isFullscreen()` / `app.exitFullscreen()` trio so the showcase exercises the canonical post-19.7 API. No HUD button — just the keyboard shortcut, matching the rest of AfterBurner's minimal-chrome aesthetic. Handler is registered + torn down inside `createGame`, so the example's React mount/unmount cycle doesn't leak duplicate listeners across remounts (which would stack up fullscreen toggles on every key press). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit (b35cb5a) ended up only landing the canvas-size tweak (1024×768 → 1024×576) — the actual F-key code I intended to ship in that commit got clobbered by an IDE save before `git add` ran. This commit adds the real change: a `KEYDOWN` subscription in `createGame` that toggles `app.requestFullscreen()` / `app.exitFullscreen()` keyed off `app.isFullscreen()`, plus a matching `event.off(...)` in the teardown function so the handler doesn't leak duplicate registrations across React mount/unmount cycles. Exercises the canonical post-19.7 app-instance fullscreen trio (consistent with the platformer + platformer-matter examples). No HUD button — keeps AfterBurner's minimal-chrome aesthetic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/melonjs/src/system/device.ts:308
- The eslint-disable rationale "reassignable via internal setters even though no setter exists today" is contradictory — if there is no setter,
prefer-constis correct, and the binding can't be reassigned from outside the module anyway (ESM exports are read-only on the consumer side). Either drop thelet+ eslint-disable and useconst, or add the missing setter function. As written,me.device.autoFocus = falsefrom user code throws in strict-mode ESM, so the apparent mutability is misleading API surface.
Two related fixes: 1. **F-key handler moved inside the preload callback.** Registered at createGame's sync tail it was getting stripped on the first React StrictMode dev unmount (utils.tsx runs teardown on every useEffect cleanup) and never re-registered because the canvas- remount path doesn't re-invoke createGame. Registering inside the preload callback puts it alongside the rest of the game- running state, surviving StrictMode the same way `audio.playTrack` does. Matches the platformer / platformer-matter pattern. 2. **HUD positions read live viewport dimensions.** `HUD.ts` hardcoded `CANVAS_W=1024 / CANVAS_H=768`; after the recent 1024×576 retune the GAME OVER overlay landed in the lower half (y=372 in a 576-tall world) and the bottom-edge credits ran off-canvas entirely (y=752). Now the constructor reads `app.viewport.width / .height` once and uses those everywhere (score, hi-score, lives, music + asset credits, GAME OVER + sub-line, DeathFlash bounds). The screen-projection ortho that drives `floating = true` renderables uses the same `viewport.width / .height` (see `Camera3d.screenProjection`), so the HUD positions track whatever Application size the example is configured with — no resize listener needed under `scale: "auto"` (default scaleMethod doesn't call `renderer.resize`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a "Powered by melonJS: https://melonjs.org" line at the bottom-center of the AfterBurner HUD, between the existing davidKBD music credit (bottom-left) and the Kenney art credit (bottom-right). All three share the size 11 / muted #bbbbbb tint so they read as one paired strip. Tag-along touch-ups: - Shorten the Kenney URL from `/assets/space-kit` to the bare `https://kenney.nl` root so the three lines balance on width. - Move all three credits from `h - 16` → `h - 6` so they sit closer to the bottom edge of the viewport (more breathing room above for gameplay, less awkward gap below). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot flagged the public export as adding a forever-supported API surface just for the spec's benefit. The function stays exported (the spec asserts the SAME predicate the module evaluates at load time — no drift), but the JSDoc now declares it as a test-seam with no stability guarantee. TypeDoc with `--excludeInternal` hides it from the generated docs; consumers reaching for it accept that the engine can change / inline / rename it without a breaking-change bump. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's `pnpm lint` flagged the missing `@param nav` on the function I added in the previous commit. Local `pnpm build` ran clean but hits a different code path (build step has stricter JSDoc enforcement than the standalone `eslint src tests` invocation CI runs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The debug panel was registered but never opened — the example's GameController binds `S` (the panel's default keyboard toggle) for WASD-down movement, leaving only the plugin's floating button as the entry point, which clutters the showcase chrome. AfterBurner's intended aesthetic is minimal HUD + uninterrupted gameplay; the platformer / debug-focused examples are where the panel belongs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
packages/melonjs/src/system/device.ts:484
device.exitFullscreenassumesdocument.exitFullscreen()exists and returns a Promise. On engines wherehasFullscreenSupportis true due to vendor-prefixed support (or whereexitFullscreenreturnsvoid), this will throw (exitFullscreen is not a functionor.catchof undefined). It should mirror the prefixed OR-chain used forrequestFullscreen()and only attach.catchwhen a Promise is returned.
packages/melonjs/src/system/device.ts:308- The inline eslint-disable rationale for
autoFocusis inaccurate: there are no internal setters here, and (per ESM namespace import semantics) this is not actually user-mutable when imported asme.device.autoFocus. This comment is misleading and should be corrected to reflect that it’s only mutable within this module / reserved for a future setter (see #1486).
Copilot caught a real asymmetry: `requestFullscreen` got the four-variant OR chain (`requestFullscreen || webkit… || moz… || ms…`) on both `Application` and the deprecated `device` wrapper, but `exitFullscreen` just called `document.exitFullscreen()` unconditionally on both. If `hasFullscreenSupport` was set via a prefixed `*FullscreenEnabled` flag (older WebKit on iOS, older Safari, IE-derived browsers), the unprefixed `document.exitFullscreen` may not exist and the call would throw — exactly the failure mode `app. requestFullscreen` was already protected against. Both `app.exitFullscreen` and the deprecated `device.exitFullscreen` now mirror the request side: probe `exitFullscreen || webkitExitFullscreen || mozCancelFullScreen || msExitFullscreen` (note the Mozilla quirk — `mozCancelFullScreen`, not `mozExitFullScreen`), guard the Promise return with `result instanceof Promise` so vendor-prefixed variants returning `void` don't trip the `.catch`. `DocumentLegacy` in `device.ts` and a new local `DocumentWithLegacyExitFullscreen` in `application.ts` carry the intersection-type declarations the four-variant probe needs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ecated.js (#1467) The two `device.*` deprecated wrappers had grown enough body (vendor- prefix probes + Promise-guards + getParent fallback) to be worth co-locating with the rest of the engine's deprecation surface. `lang/deprecated.js` now owns the function bodies and the `warning("device.requestFullscreen", ...)` / `warning("device.exit Fullscreen", ...)` runtime emit — matching the established pattern of every other entry in that file. `device.ts` re-exports them so `me.device.requestFullscreen` / `me.device.exitFullscreen` keep working for backwards compat; consumers still get the `@deprecated` JSDoc + IDE squiggle + (now) a runtime console warning the first time they call either. Side-effects of the move: - `getParent` import + the `ElementLegacy` type drop from `device.ts` (only used by the now-relocated body) - `DocumentLegacy` shrinks back to just the four-variant `fullscreenEnabled` / `fullscreenElement` typings (the `*Exit*` variants live in deprecated.js's plain-JS scope now) - `lang/deprecated.js` picks up imports from `system/device.ts` (`hasFullscreenSupport`, `isFullscreen`) and `video/video.js` (`getParent`). Imports are evaluated lazily on first call so the cycle device.ts → deprecated.js → device.ts is safe at load time. Verified via Playwright keyboard.press("KeyF") on the platformer example — still req:1 / exit:1 end-to-end through the same four- variant probe, zero page errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
packages/melonjs/src/system/device.ts:137
touch/maxTouchPointsare computed at module load usingglobalThis.navigator.maxTouchPointswithout anavigatorexistence guard. In Node/SSR (or any env wherenavigatoris missing), importingsystem/devicewill throw before any code runs.
packages/melonjs/src/system/device.ts:302- The lint-disable comment for
autoFocusclaims it's a “public mutable flag” and “reassignable via internal setters”, but there is currently no setter and no internal reassignment in this module. This is misleading (especially given ESM namespace-import write restrictions) and should be reworded to reflect the current behavior and intent.
| import { hasFullscreenSupport, isFullscreen } from "../system/device.ts"; | ||
| import CanvasRenderer from "../video/canvas/canvas_renderer.js"; | ||
| import CanvasRenderTarget from "../video/rendertarget/canvasrendertarget.js"; | ||
| import { getParent } from "../video/video.js"; |
…om device.* (#1467) Two tweaks in response to PR feedback: 1. **Circular import broken** (Copilot review on fbf04aa). `lang/deprecated.js` was importing `hasFullscreenSupport` / `isFullscreen` from `device.ts` while `device.ts` re-exported `requestFullscreen` / `exitFullscreen` from `lang/deprecated.js`. Factored both probes into a new leaf-level `system/fullscreen.ts` that owns the four-variant `DocumentLegacy` type and the detection logic; `device.ts` and `lang/deprecated.js` both import from there, eliminating the cycle. 2. **Dropped the `me.device.requestFullscreen` / `me.device.exit Fullscreen` aliases entirely.** `index.ts` already does `export * from "./lang/deprecated.js"`, so the two function names surface at the top level as `me.requestFullscreen()` / `me.exitFullscreen()` — a deprecated shim with runtime warning that points at `Application#requestFullscreen` / `Application#exitFullscreen`. Re-exporting them under `me.device.*` defeated the whole point of "they live in deprecated.js" — it leaked the deprecated names back into the namespace they were moved out of. Users still calling `me.device.requestFullscreen()` directly need to either migrate to `app.requestFullscreen()` (canonical) or drop the `.device.` segment. `device.ts` keeps `hasFullscreenSupport` and `isFullscreen` exposed under `me.device.*` via the new re-export from `system/fullscreen.ts` — those are non-deprecated probes and remain valid public API. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…export Reverts the unintended removal of the device-namespace aliases from fd0fc67. Per the user clarification: implementations stay in `lang/deprecated.js` (correct — that's where the engine's other deprecation surface lives) but the names need to be reachable as `me.device.requestFullscreen` / `me.device.exitFullscreen` for backwards compatibility with consumers still calling through that namespace. Net structure: - `lang/deprecated.js` — function bodies + `warning(...)` runtime call + the four-variant vendor probe - `system/device.ts` — one re-export line so the names appear on `me.device.*`. eslint-disable on the re-export covers the `no-deprecated` rule (which fires when re-exporting deprecated symbols — intentional here, since the whole point is to surface them under the deprecated alias) - `system/fullscreen.ts` — `hasFullscreenSupport` / `isFullscreen` (canonical, non-deprecated), imported by BOTH device.ts and lang/deprecated.js. No circular dep. Verified round-trip via Playwright keypress on the platformer example — still req:1 / exit:1 end-to-end through the four-variant probe, zero page errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/melonjs/src/system/device.ts:289
- The new
prefer-constsuppression comment forautoFocusis misleading: there are no internal setters (or any reassignment in this module), so the current comment claims a mutability mechanism that doesn’t exist. This makes it harder to understand the real status ofautoFocus(see #1486) and why it’s intentionally alet.
| /** | ||
| * iPad-on-Mac-UA predicate. Exported so the spec file can assert the | ||
| * SAME function the module evaluates at load time (no drift between | ||
| * docs and implementation), but marked `@internal` because it's a | ||
| * test-seam, not a stable public API — the engine reserves the right | ||
| * to change / inline / rename it without a breaking-change bump. | ||
| * @param nav - a `navigator`-shaped object (or `undefined` for Node/SSR) | ||
| * @returns `true` when `nav` looks like an iPad reporting under the iPadOS-13+ desktop Mac UA | ||
| * @internal | ||
| */ | ||
| export function isIPadOnMacUA(nav: NavigatorLike | undefined): boolean { | ||
| return nav?.platform === "MacIntel" && (nav?.maxTouchPoints ?? 0) > 1; | ||
| } |
…types Copilot caught that `tsconfig.build.json` doesn't set `stripInternal`, so the `@internal` JSDoc on `isIPadOnMacUA` doesn't actually prevent the emitted `.d.ts` from carrying the function (and its parameter type) into the public surface. The named `NavigatorLike` alias would have shown up in the published types as a brand-new engine-defined type — making any later rename / removal a breaking change. Inlined the navigator shape as `Partial<Pick<Navigator, "platform" | "maxTouchPoints">> | undefined` so only DOM-lib types appear in the signature. The `@internal` tag still flags the predicate as test-seam-only for TypeDoc (`--exclude Internal`), and the contract under test is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #1467.
Scope grew during review from the original "iPadOS 13+ detection + dead-platform deprecation" into a broader platform/device cleanup. Updated to reflect what actually ships.
What this fixes / changes
1. Modern iPads (iPadOS 13+, since Sept 2019) report as desktop
Safari on iPad ships the desktop Mac UA — no
iPadtoken. The/iPhone|iPad|iPod/iregex missed every iPad sold in the last ~7 years; they all fell throughisMobileas desktop. Every internal consumer (keyboard.ts,application.ts,header.ts) was getting the wrong answer for ~30% of mobile traffic.Fix: layer a feature check on top of the UA regex:
navigator.platform === "MacIntel"is NOT a CPU check — Apple deliberately freezes the legacy string for backwards compat (same trick Windows uses withWin32on 64-bit). Apple Silicon Macs / iPads (M1, M2, M3, M4) all reportMacIntel. ThemaxTouchPoints > 1clause is what actually separates iPads from real Macs.isIPadOnMacUA(nav)is extracted as a@internal-tagged exported function so the spec asserts the SAME predicate the module evaluates at load time (drift-proof). Marked@internalbecause it's a test-seam, not stable API.2. Dead-platform regex noise
@deprecated since 19.7.0onwp/BlackBerry/Kindle/android2. Exports stay functional through 19.x for backwards compat; IDE warnings light up at call sites. Removal scheduled for 20.x.Also dropped these from
isMobile's OR chain — remaining/Mobi/.test(ua) || iOS || androidcovers ~99.9% of 2026 mobile traffic per MDN.3.
system/device.js→system/device.ts(945 lines)Mechanical conversion — JSDoc was already exhaustive, so most types just become native TS annotations. Non-standard / legacy browser surfaces (
Document.mozFullScreenEnabled,Navigator.standalone, iOS-onlyDeviceOrientationEvent.requestPermission, etc.) typed via narrow local intersection types. Two small correctness improvements that fell out:onDeviceMotionnow guards againstaccelerationIncludingGravity === nullgetElementJSDoc corrected (function never returns null, always falls back todocument.body)4.
Application#requestFullscreen/exitFullscreen/isFullscreen— fullscreen finally has app-instance contextdevice.requestFullscreen()had to fall back to the deprecatedgetParent()→game.getParentElement()global-game lookup because the static helper had no Application reference. The new methods onApplicationusethis.parentElementdirectly — canvas + sibling HUD go fullscreen together, no deprecated chain.device.requestFullscreen/device.exitFullscreendeprecated (since 19.7.0) pointing at the Application methods.device.isFullscreenstays non-deprecated because it's a stateless document-state probe (there's exactly one fullscreen state per document regardless of how many Applications run);Application#isFullscreenis a thin convenience.Also cleaned up the underlying probe in the deprecated wrappers — replaced
prefixed("fullscreenElement", document as unknown as Record<string, unknown>)etc. with an explicit four-variant OR chain (the pattern lib.dom.d.ts itself uses).5.
keyboard.ts— drop theif (!isMobile)gateInitially marked as a follow-up but pulled into this PR after the iPad fix made the gate observably wrong. The gate assumed "mobile = no physical keyboard" — invalid for iPads (now correctly identified) with Magic Keyboard, Samsung DeX, ChromeOS tablet mode, Bluetooth-keyboard-on-phone. Two empty listener slots cost nothing on touch-only devices; the unbound-key path is a single map lookup that returns undefined.
Also changed the surrounding
if (globalThis.addEventListener)totypeof globalThis.addEventListener === "function"per Copilot's defensive-style suggestion.6. Example migrations
platformer/createGame.ts,platformer/HUD.ts,platformer-matter/createGame.ts,platformer-matter/HUD.tsmigrated fromdevice.{is,request,exit}Fullscreen()toapp.*/game.*(canonical post-19.7 API).afterBurner/ExampleAfterBurner.tsx— added F→fullscreen shortcut + canvas retuned 1024×768 → 1024×576 (16:9, suits widescreen fullscreen better).afterBurner/HUD.ts— refactored to read liveapp.viewport.width / .heightso HUD positions follow the configured Application size (was previously hardcoded to the old 1024×768). Added "Powered by melonJS" credit and tightened the bottom credit strip.Won't-add:
isTouchOriginal issue suggested a new
isTouchflag. We already havedevice.touchatsystem/device.ts— feature-detected vianavigator.maxTouchPoints/ pointer events. CHANGELOG migration note points there.Tests
Six new cases in
tests/platform.spec.tscovering the documented iPad-detection contract:platform=MacIntel, maxTouchPoints=5) → flaggedplatform=MacIntel, maxTouchPoints=0) → not flaggedmaxTouchPointsundefined (older Safari) → not flaggedplatform=Win32, maxTouchPoints=10) → not flaggedmaxTouchPoints === 1(single-point touch edge case) → not flagged (> 1excludes it)Existing 20 shape / desktop-defaults assertions kept.
Test plan
pnpm test:typescleanpnpm vitest run— 3975 / 13 skipped / 0 failed (was 3969, +6 new)pnpm build— lint + types cleanFollow-ups (separate tickets)
Filed out of scope:
device.autoFocusislet-declared and documented as user-settable, but ESM namespace-import bindings are read-only externally. Needs a proper setter.navigator.userAgentDatawhere available. Chromium-only today; Safari/Firefox lag. 20.x candidate.🤖 Generated with Claude Code